Skip to content

Add favicon when logo option is present #774

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 10, 2017
Merged

Add favicon when logo option is present #774

merged 1 commit into from
Sep 10, 2017

Conversation

gfvcastro
Copy link
Contributor

No description provided.

@josevalim josevalim merged commit cb94dee into elixir-lang:master Sep 10, 2017
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@gfvcastro gfvcastro deleted the add-favicon-when-logo-option branch September 10, 2017 15:38
@ericmj
Copy link
Member

ericmj commented Oct 2, 2017

If the logo is not centered and inside a square image it will look a bit weird. Take a look at https://hexdocs.pm/phoenix and https://hexdocs.pm/ecto. Should we document this and fix the projects or is there some other way to solve it?

@josevalim
Copy link
Member

Yup, we should probably have something to be exclusively the favicon.

josevalim pushed a commit that referenced this pull request Oct 3, 2017
Revert "Add favicon when logo option is present (#774)"
1998595 
This reverts commit cb94dee.
@gfvcastro
Copy link
Contributor Author

Good catch! If you want, I can send a new PR. 😄

@josevalim
Copy link
Member

@gfvcastro I actually don't think anything is necessary. If people have a favicon file in the output directory that is enough, right?

@ericmj
Copy link
Member

ericmj commented Oct 3, 2017 •
edited
Loading

@josevalim If you don't specify a favicon in the <head> I believe it will be fetched from the root which obviously wont work for packages.

@josevalim
Copy link
Member

Good point. Any ideas on how to solve this though? Should we have a favicon configuration like we do for the logo?

@gfvcastro
Copy link
Contributor Author

@josevalim I think it is a good solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gfvcastro @josevalim @ericmj